Conversation
📝 WalkthroughWalkthroughRelease 0.1.6 standardizes builder names, adds AST parsing, enables runtime packrat toggling, supports unlimited Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
72-72: Inconsistent test counts in documentation.Line 72 states "14 tests for grammar parser" but line 284 states "Grammar Parser Tests (17 tests)". With 3 new validation tests added, the count on line 72 should be updated to 17.
🔎 Proposed fix
-│ └── GrammarParserTest.java # 14 tests for grammar parser +│ └── GrammarParserTest.java # 17 tests for grammar parser
🧹 Nitpick comments (2)
src/main/java/org/pragmatica/peg/action/ActionCompiler.java (1)
95-108: LGTM! Unlimited $N support via regex.The regex-based approach correctly enables unlimited positional action variables as described in the PR objectives.
🔎 Optional: Remove redundant n == 0 check
Line 104's
n == 0check is unreachable because$0is already replaced withsv.token()on line 99, so the regex won't match it. You can simplify:private String transformActionCode(String code) { // Replace $0 with sv.token() var result = code.replace("$0", "sv.token()"); // Replace $N (N > 0) with sv.get(N-1) using regex for unlimited support result = POSITIONAL_VAR.matcher(result).replaceAll(match -> { int n = Integer.parseInt(match.group(1)); - return n == 0 ? "sv.token()" : "sv.get(" + (n - 1) + ")"; + return "sv.get(" + (n - 1) + ")"; }); return result; }src/main/java/org/pragmatica/peg/generator/ParserGenerator.java (1)
1385-1395: Consider adding a more descriptive rule name for error nodes.The
defaultcase intoAsthandlesCstNode.Errornodes by returning a terminal with rule name "error" and empty text. While this is a reasonable fallback, the CST error node contains useful information (skippedText,expected) that is lost.If error recovery is used and the AST is later traversed, consumers won't know what text was skipped or what was expected.
🔎 Possible enhancement to preserve error context
- default -> new AstNode.Terminal(cst.span(), "error", ""); + case CstNode.Error err -> new AstNode.Terminal(err.span(), "error", err.skippedText());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.mdCLAUDE.mdREADME.mddocs/ERROR_RECOVERY.mdpom.xmlsrc/main/java/org/pragmatica/peg/PegParser.javasrc/main/java/org/pragmatica/peg/action/ActionCompiler.javasrc/main/java/org/pragmatica/peg/generator/ParserGenerator.javasrc/main/java/org/pragmatica/peg/grammar/Grammar.javasrc/main/java/org/pragmatica/peg/parser/ParsingContext.javasrc/main/java/org/pragmatica/peg/parser/PegEngine.javasrc/test/java/org/pragmatica/peg/ErrorRecoveryTest.javasrc/test/java/org/pragmatica/peg/PegParserTest.javasrc/test/java/org/pragmatica/peg/examples/ErrorRecoveryExample.javasrc/test/java/org/pragmatica/peg/grammar/GrammarParserTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/org/pragmatica/peg/grammar/GrammarParserTest.java (1)
src/main/java/org/pragmatica/peg/grammar/GrammarParser.java (1)
GrammarParser(16-542)
🔇 Additional comments (21)
src/main/java/org/pragmatica/peg/parser/ParsingContext.java (3)
25-25: LGTM! Stable cache keys via ruleIds.Using a dedicated map to assign numeric IDs to rule names provides deterministic cache keys, addressing hash collision risks mentioned in the PR objectives.
Also applies to: 50-50
42-43: LGTM! Thread-safe whitespace skipping guard.Moving whitespace skipping state into ParsingContext with explicit enter/exit lifecycle methods improves thread safety and prevents recursive whitespace parsing.
Also applies to: 279-291
347-350: LGTM! Improved packrat key generation.The new approach using
ruleIds.computeIfAbsentensures each rule gets a stable, unique numeric ID, then packs it with the position into a long. This is more reliable than hashCode-based keys.src/main/java/org/pragmatica/peg/parser/PegEngine.java (2)
878-909: LGTM! Context-driven whitespace handling.The refactoring moves whitespace skipping state from a local field to ParsingContext, improving thread safety. The enter/exit lifecycle ensures proper cleanup.
915-922: LGTM! Helper for trivia collection.The extractInnerExpression helper correctly unwraps repetition operators (ZeroOrMore, OneOrMore, Optional) to allow matching whitespace elements one at a time for proper trivia collection.
src/test/java/org/pragmatica/peg/PegParserTest.java (1)
268-275: LGTM! Builder API rename applied.The update from
withPackrat(true)topackrat(true)aligns with the standardized builder API naming mentioned in the PR objectives.docs/ERROR_RECOVERY.md (1)
35-35: LGTM! Documentation updated for API rename.The examples now correctly use
.recovery(RecoveryStrategy)instead of.withErrorRecovery(RecoveryStrategy), consistent with the builder API standardization.Also applies to: 48-48, 61-61, 77-77, 409-409, 412-412, 415-415, 486-486, 528-528, 556-556
src/main/java/org/pragmatica/peg/grammar/Grammar.java (2)
53-67: LGTM! Grammar validation for undefined references.The validate() method now detects undefined rule references as mentioned in the PR objectives. It properly returns a SemanticError with the rule name and location when an undefined reference is found.
69-110: LGTM! Comprehensive expression traversal.The findUndefinedReference helper correctly handles all expression types, including sequences, choices, repetitions, and nested structures. Terminal expressions appropriately return
Option.none().src/test/java/org/pragmatica/peg/examples/ErrorRecoveryExample.java (1)
46-46: LGTM! Test examples updated for API rename.All test methods now use
.recovery(RecoveryStrategy)instead of.withErrorRecovery(RecoveryStrategy), consistent with the builder API standardization.Also applies to: 63-63, 77-77, 138-138, 208-208, 233-233, 250-250, 277-277, 304-304
src/test/java/org/pragmatica/peg/grammar/GrammarParserTest.java (1)
185-226: LGTM! Solid test coverage for grammar validation.The three new tests appropriately cover:
- Direct undefined rule references
- Valid grammar (positive case)
- Nested undefined references within complex expressions
The test structure follows the existing patterns in the file and the assertions clearly verify the expected behavior.
src/test/java/org/pragmatica/peg/ErrorRecoveryTest.java (1)
47-47: LGTM! Builder API consistently updated.All usages of
.withErrorRecovery(...)have been correctly renamed to.recovery(...)across the test file, aligning with the standardized builder API naming convention.src/main/java/org/pragmatica/peg/PegParser.java (1)
134-147: LGTM! Builder API naming standardization.The method renames from
withX()toX()follow a more concise builder pattern. This is a breaking change that is properly documented in the CHANGELOG.CLAUDE.md (1)
174-176: LGTM! API examples correctly updated.The configuration examples now reflect the standardized builder method names (
packrat(),trivia(),recovery()).README.md (1)
25-25: LGTM! Version and documentation correctly updated.The version bump to 0.1.6 and all API examples have been updated to reflect the standardized builder method names.
CHANGELOG.md (1)
8-51: LGTM! Comprehensive and well-structured changelog entry.The release notes clearly document:
- New features (AST support, packrat toggle, unlimited action variables)
- Bug fixes (grammar validation, thread safety, cache key collision)
- Breaking changes (API renames) with migration path
This follows the Keep a Changelog format and provides good context for users upgrading.
src/main/java/org/pragmatica/peg/generator/ParserGenerator.java (5)
196-212: LGTM! Packrat memoization toggle correctly implemented.The implementation properly:
- Defaults to enabled (preserving existing behavior)
- Initializes cache as
nullwhen disabled to avoid memory allocation- Guards all cache operations with null checks
294-302: LGTM! Cache lookup correctly guarded.The null check ensures no NPE when packrat is disabled.
752-761: LGTM! Unlimited action variable support.The regex-based replacement correctly:
- Uses a compiled
Patternfor efficiency- Handles
$0specially (preserving it for matched text)- Converts 1-based
$Nto 0-basedvalues.get(N-1)
951-957: LGTM! Clean AstNode interface design.The sealed interface with
TerminalandNonTerminalrecords provides a simple, immutable AST representation.
1470-1476: LGTM! CST cache lookup consistently guarded.Mirrors the non-CST implementation correctly.
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <maven.compiler.release>25</maven.compiler.release> | ||
| <pragmatica-lite.version>0.8.4</pragmatica-lite.version> | ||
| <pragmatica-lite.version>0.9.0</pragmatica-lite.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the pom.xml file to understand the context
git ls-files "*pom.xml*" | head -5Repository: siy/java-peglib
Length of output: 66
🏁 Script executed:
# Search for pragmatica-lite in pom.xml and surrounding lines
rg "pragmatica-lite" -A 5 -B 5Repository: siy/java-peglib
Length of output: 19372
🏁 Script executed:
# Check Maven Central for pragmatica-lite 0.9.0 availability
curl -s "https://search.maven.org/solrsearch/select?q=g:org.pragmatica-lite+AND+a:core+AND+v:0.9.0&rows=1&wt=json" | jq '.'Repository: siy/java-peglib
Length of output: 501
🏁 Script executed:
# Check for repository configurations in pom.xml
cat pom.xml | head -100Repository: siy/java-peglib
Length of output: 3379
🏁 Script executed:
# Check if pragmatica-lite 0.8.4 exists (previous version)
curl -s "https://search.maven.org/solrsearch/select?q=g:org.pragmatica-lite+AND+a:core&rows=10&wt=json" | jq '.response.docs[] | {v, timestamp}'Repository: siy/java-peglib
Length of output: 41
🏁 Script executed:
# Check if there are any repository configurations
rg "repositories|url" pom.xml -A 2Repository: siy/java-peglib
Length of output: 408
pragmatica-lite:core 0.9.0 is not available in Maven Central.
The dependency declared on line 41 cannot be resolved. Before releasing 0.1.6, either publish pragmatica-lite:core 0.9.0 to Maven Central or adjust the version to an available release (e.g., 0.8.4).
🤖 Prompt for AI Agents
In pom.xml around line 41, the pragmatica-lite.version property is set to 0.9.0
which is not available on Maven Central; either change that property to a
published version (e.g., 0.8.4) in the pom.xml and run a full build (mvn -U
clean verify) to ensure dependency resolution, or publish pragmatica-lite:core
0.9.0 to Maven Central following our release process and then leave the pom
unchanged.
|
Re: pragmatica-lite:core 0.9.0 availability The CI build passed successfully, which confirms that The CodeRabbit check may have been performed before the artifact was fully propagated across Maven Central mirrors, or used cached search results. No action required - the dependency is available and the build works. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
408-408: Update stale test count in build commands.Line 408 references 268 passing tests, but the document states 271 tests overall (line 142 and 283). Update this comment to reflect the current test count.
🔎 Proposed fix
-mvn test # Run tests (268 passing) +mvn test # Run tests (271 passing)
- Consolidated 3 duplicate expression parsing switch statements into unified parseExpressionWithMode() - Extracted buildParseError() helper to eliminate duplicate error message construction - Removed unused SemanticValues.choice field and getter - Removed unused SourceLocation.advanceColumn()/advanceLine() methods - Removed unused ParserConfig.Builder - ~107 lines reduced in PegEngine.java 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/pragmatica/peg/parser/PegEngine.java (1)
821-842: Consider a more robust approach for distinguishing custom error messages.The heuristic at lines 836-840 uses string prefix matching to differentiate custom error messages from standard ones. While this works with current message patterns, it's fragile and could break if:
- A custom message happens to start with
',[, or other checked prefixes- New standard message formats are introduced that don't match these patterns
- The list of checked patterns needs to be maintained as the grammar evolves
A more maintainable approach would be to explicitly mark custom messages using a flag or wrapper type (e.g.,
ParseResult.Failure.withCustomMessage(...)) rather than inferring from string content.💡 Alternative approach using explicit marking
Instead of inferring from string patterns, you could:
- Add a field to
ParseResult.Failureto indicate custom messages:record Failure(SourceLocation location, String expected, boolean isCustomMessage) implements ParseResult { ... }
- Then simplify the logic:
var expected = failureExpected instanceof Failure f && f.isCustomMessage() ? failureExpected.expected() : ctx.furthestExpected();This makes the intent explicit and eliminates pattern-matching fragility.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdCLAUDE.mdsrc/main/java/org/pragmatica/peg/action/SemanticValues.javasrc/main/java/org/pragmatica/peg/parser/ParserConfig.javasrc/main/java/org/pragmatica/peg/parser/PegEngine.javasrc/main/java/org/pragmatica/peg/tree/SourceLocation.java
💤 Files with no reviewable changes (2)
- src/main/java/org/pragmatica/peg/parser/ParserConfig.java
- src/main/java/org/pragmatica/peg/tree/SourceLocation.java
🔇 Additional comments (10)
src/main/java/org/pragmatica/peg/action/SemanticValues.java (4)
10-10: LGTM! Clean removal of unused choice tracking.The removal of the
choicefield is consistent throughout the file—constructor, factory method, and toString are all updated. The semantic values API is now simpler and focused on matched text and child values.Also applies to: 18-26, 136-136
77-77: Excellent centralization of error construction logic.The new
buildParseError()helper eliminates duplicate error message construction acrossparseCst()andparse()methods. The logic correctly uses the furthest location tracked during backtracking (ctx.furthestLocation(),ctx.furthestPos(),ctx.furthestExpected()) to provide accurate error positions and messages.Also applies to: 134-134, 821-842
778-778: Good improvement to thread safety for whitespace skipping.Moving the whitespace-skipping guard from a per-instance field to per-parse context methods (
isSkippingWhitespace(),enterWhitespaceSkip(),exitWhitespaceSkip()) eliminates potential race conditions when parser instances are reused across threads. The try-finally block ensures proper cleanup even if parsing fails.Also applies to: 782-803
293-293: Excellent consolidation of duplicate expression parsing logic.The unified
parseExpressionWithMode()dispatcher withParseMode(standard, withActions, noWhitespace) eliminates significant code duplication. All call sites are consistently updated to use the mode-aware dispatcher, making the code more maintainable and easier to extend.Also applies to: 326-326, 386-386, 406-406, 422-422, 662-662, 673-673, 691-691, 710-710, 721-721, 736-736, 1159-1193
CLAUDE.md (1)
21-21: Documentation accurately reflects API and dependency changes.The updates correctly document:
- Dependency upgrade to pragmatica-lite:core 0.9.0
- Builder API method renames (
.packrat(),.trivia(),.recovery())- Test count increase to 271
All documentation changes are consistent with the code modifications.
Also applies to: 133-133, 176-178, 252-252
CHANGELOG.md (1)
8-58: Excellent release documentation with comprehensive technical details.The 0.1.6 changelog entry is well-structured and thorough:
- Added section documents new features (AST support, packrat toggle, unlimited $N variables)
- Fixed section highlights important bug fixes (grammar validation, thread safety, cache key collisions)
- Changed section covers API improvements and code simplifications (~120 lines eliminated)
The specific technical details (e.g., "cache key from hashCode() to unique sequential IDs") make it easy to understand the impact of each change. The format follows Keep a Changelog conventions.
src/main/java/org/pragmatica/peg/parser/PegEngine.java (4)
77-77: LGTM: Centralized error building improves consistency.The refactoring to use
buildParseErrorconsolidates error construction logic, making it easier to maintain consistent error messages across different parsing paths.Also applies to: 134-134
775-806: LGTM: Whitespace lifecycle management prevents recursion and ensures cleanup.The addition of
isSkippingWhitespace()check at line 778 prevents infinite recursion when the whitespace grammar itself needs whitespace parsing. The try-finally block (lines 782-803) ensuresexitWhitespaceSkip()is always called, even if parsing throws an exception.According to the PR objectives, moving whitespace state into
ParsingContextalso addresses thread safety concerns since each parse creates its own context.
293-293: LGTM: Mode-aware parsing consolidates duplicate code effectively.The refactoring replaces three separate methods (
parseExpression,parseExpressionWithActions,parseExpressionNoWs) with a unifiedparseExpressionWithModedispatcher. Mode selection is appropriate across all call sites:
standard()for normal CST parsing (lines 293, 662, 673, 691, 710, 721, 736)withActions()when collecting semantic values and executing actions (lines 326, 386, 406, 422)noWhitespace()when parsing whitespace itself (line 791) — critical for preventing infinite recursionThis consolidation reduces code duplication and improves maintainability.
Also applies to: 326-326, 386-386, 406-406, 422-422, 662-662, 673-673, 691-691, 710-710, 721-721, 736-736, 791-791
1158-1162: LGTM: Clear documentation of the unified parsing method.The comment accurately describes the three parsing modes (
standard,withActions,noWhitespace) supported by the consolidated method.
1 participant
Summary
Release 0.1.6 with quality improvements from comprehensive code review.
Added
AstNodetype +parseAst()method)setPackratEnabled(boolean))$Naction variable support (regex-based, removes $1-$20 limit)Fixed
skippingWhitespacemoved toParsingContexthashCode())Changed
withPackrat()→packrat(), etc.%worddirective)Test plan
mvn verifysucceeds🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Changed
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.